Skip to content

Math: Library: Add the hifi4 exponential function#6801

Merged
kv2019i merged 1 commit into
thesofproject:mainfrom
ShriramShastry:Hifi4_Exponential_function_dev
Jun 16, 2023
Merged

Math: Library: Add the hifi4 exponential function#6801
kv2019i merged 1 commit into
thesofproject:mainfrom
ShriramShastry:Hifi4_Exponential_function_dev

Conversation

@ShriramShastry
Copy link
Copy Markdown
Contributor

@ShriramShastry ShriramShastry commented Dec 14, 2022 •
edited
Loading

The 32-bit HiFi4 exponential library function has an accuracy of 1e-4, a unit in last place error of 5.60032793, and output ranges from 0.0067379470 to 148.4131591026 for inputs from -5 to +5 (Q4.28) (Q9.23).

Signed-off-by: ShriramShastry malladi.sastry@intel.com

image

@ShriramShastry ShriramShastry force-pushed the Hifi4_Exponential_function_dev branch 7 times, most recently from 3ed16a6 to 131fc13 Compare December 14, 2022 14:19
lgirdwood
lgirdwood reviewed Dec 15, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrula-song @singalsu pls review

Comment thread src/math/Kconfig Outdated
singalsu
singalsu reviewed Dec 19, 2022
View reviewed changes
Comment thread src/math/exp_fcn_hifi4.c Outdated
Copy link
Copy Markdown
Collaborator

@singalsu singalsu Dec 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra blanks after ae_int64 *. You could also avoid this function proto with move of mul_s64() here.

Comment thread src/math/exp_fcn_hifi4.c Outdated
Comment thread src/math/exp_fcn_hifi4.c Outdated
Comment thread src/math/exp_fcn_hifi4.c Outdated
Comment thread src/math/exp_fcn_hifi4.c Outdated
Copy link
Copy Markdown
Collaborator

@singalsu singalsu Dec 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

declare variables in the begin of function

Copy link
Copy Markdown
Contributor Author

@ShriramShastry ShriramShastry Dec 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

Comment thread src/math/exp_fcn_hifi4.c Outdated
Comment thread src/math/exp_fcn_hifi4.c Outdated
Copy link
Copy Markdown
Collaborator

@singalsu singalsu Dec 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declare all variables in the beginning

Copy link
Copy Markdown
Contributor Author

@ShriramShastry ShriramShastry Dec 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

Comment thread src/math/exp_fcn_hifi4.c Outdated
Comment thread src/math/exp_fcn_hifi4.c Outdated
Copy link
Copy Markdown
Collaborator

@singalsu singalsu Dec 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you compare qt directly to integer constant without need for qt_temp?

Copy link
Copy Markdown
Contributor Author

@ShriramShastry ShriramShastry Dec 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qt temp = qt; is used to prevent the following error in the event that (qt < CONVERG_ERROR)
no operator overload defined for binary operator '<' (operand types 'ae_int64' (aka '_TIE_xt_hifi2_ae_int64') 'long long')
can you please suggest clean approach.

Comment thread test/cmocka/src/math/arithmetic/exponential_hifi4.c Outdated
@ShriramShastry ShriramShastry force-pushed the Hifi4_Exponential_function_dev branch 17 times, most recently from 51bc618 to 3465959 Compare December 27, 2022 06:55
@ShriramShastry ShriramShastry force-pushed the Hifi4_Exponential_function_dev branch from 3465959 to a771b4a Compare January 3, 2023 15:31
ShriramShastry
ShriramShastry commented Jan 3, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@ShriramShastry ShriramShastry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for looking at the code.
I incorporated the proposal and expressed my opinion.

Comment thread src/math/Kconfig Outdated
Comment thread src/math/exp_fcn_hifi4.c Outdated
Comment thread src/math/exp_fcn_hifi4.c Outdated
Comment thread src/math/exp_fcn_hifi4.c Outdated
Comment thread test/cmocka/src/math/arithmetic/exponential_hifi4.c Outdated
Comment thread src/math/exp_fcn_hifi4.c Outdated
Comment thread src/math/exp_fcn_hifi4.c Outdated
Copy link
Copy Markdown
Contributor Author

@ShriramShastry ShriramShastry Dec 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qt temp = qt; is used to prevent the following error in the event that (qt < CONVERG_ERROR)
no operator overload defined for binary operator '<' (operand types 'ae_int64' (aka '_TIE_xt_hifi2_ae_int64') 'long long')
can you please suggest clean approach.

Comment thread src/math/exp_fcn_hifi4.c Outdated
Copy link
Copy Markdown
Contributor Author

@ShriramShastry ShriramShastry Dec 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

Comment thread src/math/exp_fcn_hifi4.c Outdated
Copy link
Copy Markdown
Contributor Author

@ShriramShastry ShriramShastry Dec 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

@ShriramShastry ShriramShastry force-pushed the Hifi4_Exponential_function_dev branch from a771b4a to 8eab8d6 Compare January 4, 2023 06:08
@lgirdwood
Copy link
Copy Markdown
Member

lgirdwood commented Jan 4, 2023

@ShriramShastry btw, whats the speedup here vs generic C version ?

@ShriramShastry ShriramShastry force-pushed the Hifi4_Exponential_function_dev branch 2 times, most recently from 9533b89 to cacde9d Compare June 3, 2023 02:02
cujomalainey
cujomalainey reviewed Jun 6, 2023
View reviewed changes
Comment thread src/math/exp_fcn_hifi4.c Outdated
singalsu
singalsu requested changes Jun 9, 2023
View reviewed changes
Comment thread src/include/sof/math/exp_fcn_hifi4.h Outdated
Comment thread src/include/sof/math/exp_fcn_hifi4.h Outdated
Comment thread src/include/sof/math/exp_fcn_hifi4.h Outdated
Comment thread src/math/CMakeLists.txt Outdated
Comment thread test/cmocka/src/math/arithmetic/exponential.c Outdated
Copy link
Copy Markdown
Collaborator

@singalsu singalsu Jun 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unrelated change could be in another commit with title indicating improvement of cmocka test coverage. I assume the old test with 256 points would work with hifi4 version too.

@ShriramShastry ShriramShastry force-pushed the Hifi4_Exponential_function_dev branch from cacde9d to 954d65f Compare June 9, 2023 09:13
singalsu
singalsu reviewed Jun 9, 2023
View reviewed changes
Comment thread test/cmocka/src/math/arithmetic/exponential.c Outdated
@ShriramShastry ShriramShastry force-pushed the Hifi4_Exponential_function_dev branch 6 times, most recently from 04ebea6 to b4c6345 Compare June 9, 2023 16:21
@ShriramShastry ShriramShastry requested a review from singalsu June 9, 2023 16:50
kv2019i
kv2019i reviewed Jun 12, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks ready to go, but the Kconfig addition/rename needs clarification. See comment inline.

Comment thread src/math/CMakeLists.txt Outdated
@ShriramShastry
Copy link
Copy Markdown
Contributor Author

ShriramShastry commented Jun 12, 2023

Code looks ready to go, but the Kconfig addition/rename needs clarification. See comment inline.

Kconfig, is enabled or disabled by below code, In the moment, it is disabled, to enable make default n to y

config EXP_FIXED
bool "Exponential functions"
default n
help
By selecting this, the 32-bit sofm_exp_int32() function can be used to calculate
exponential values. With a maximum ulp of 5, an exponential function with
an input range of -5 to +5 gives positive numbers between 0.00673794699908547 and
148.413159102577. The precision of this function is 1e-4.

@ShriramShastry ShriramShastry force-pushed the Hifi4_Exponential_function_dev branch from b4c6345 to 751f887 Compare June 12, 2023 12:07
@ShriramShastry
Copy link
Copy Markdown
Contributor Author

ShriramShastry commented Jun 12, 2023

Code looks ready to go, but the Kconfig addition/rename needs clarification. See comment inline.

Kconfig, is enabled or disabled by below code, In the moment, it is disabled, to enable make default n to y

config EXP_FIXED bool "Exponential functions" default n help By selecting this, the 32-bit sofm_exp_int32() function can be used to calculate exponential values. With a maximum ulp of 5, an exponential function with an input range of -5 to +5 gives positive numbers between 0.00673794699908547 and 148.413159102577. The precision of this function is 1e-4.

Done !! Renamed EXP_FIXED to MATH_EXP.
Thanks

kv2019i
kv2019i approved these changes Jun 12, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ShriramShastry , looks good now.

@ShriramShastry
Copy link
Copy Markdown
Contributor Author

ShriramShastry commented Jun 14, 2023

Thanks @ShriramShastry , looks good now.

Thank you. Is there any other unresolved issue that needs to be addressed?

@kv2019i
Copy link
Copy Markdown
Collaborator

kv2019i commented Jun 14, 2023

@cujomalainey Ok to proceed with this? You still have a requested-changes pending.

@cujomalainey
Copy link
Copy Markdown
Contributor

cujomalainey commented Jun 14, 2023

@cujomalainey Ok to proceed with this? You still have a requested-changes pending.

I'll re-review today

@cujomalainey
Copy link
Copy Markdown
Contributor

cujomalainey commented Jun 15, 2023

looks like my comment is still open, would be nice to get it fixed but i won't block on it

@ShriramShastry
Math: Library: Add the hifi4 exponential function
d0e3d00 
The 32-bit HiFi4 exponential library function has an accuracy of 1e-4,
a unit in last place error of 5.60032793, and output ranges from
0.0067379470 to 148.4131591026 for inputs from -5 to +5 (Q4.28) (Q9.23).

Signed-off-by: ShriramShastry <malladi.sastry@intel.com>
@ShriramShastry
Copy link
Copy Markdown
Contributor Author

ShriramShastry commented Jun 15, 2023

looks like my comment is still open, would be nice to get it fixed but i won't block on it

Done !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@singalsu singalsu singalsu approved these changes

@kv2019i kv2019i kv2019i approved these changes

@aborisovich aborisovich Awaiting requested review from aborisovich

@plbossart plbossart Awaiting requested review from plbossart plbossart is a code owner

@mmaka1 mmaka1 Awaiting requested review from mmaka1 mmaka1 is a code owner

@lbetlej lbetlej Awaiting requested review from lbetlej lbetlej is a code owner

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

@mwasko mwasko Awaiting requested review from mwasko

@kfrydryx kfrydryx Awaiting requested review from kfrydryx

@johnylin76 johnylin76 Awaiting requested review from johnylin76

@lgirdwood lgirdwood Awaiting requested review from lgirdwood lgirdwood is a code owner

@andrula-song andrula-song Awaiting requested review from andrula-song

@btian1 btian1 Awaiting requested review from btian1

+1 more reviewer

@cujomalainey cujomalainey cujomalainey approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@ShriramShastry @lgirdwood @singalsu @gkbldcig @kv2019i @cujomalainey @btian1 @andrula-song